Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow create upsert actions in state transitions #71

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

enoonan
Copy link
Contributor

@enoonan enoonan commented Nov 25, 2024

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

Updated AshStateMachine.Verifiers.VerifyTransitionActions to allow create transition actions when upsert? true is set.

Added tests and added consolidate_protocols: Mix.env() != :test to AshStateMachine.MixProject.project to avoid some unnecessary compiler warnings when testing.

@zachdaniel
Copy link
Contributor

Another place that will need to be updated is this code: https://github.com/ash-project/ash_state_machine/blob/main/lib/ash_state_machine.ex#L140

Currently, we just check that the target state is in valid initial states. However, if the action is an upsert action, we now have to look up the valid next states for all transitions for this action and make sure that the given state is in that list.

Additionally, we have to ensure at compile time that a state transition listed for an upsert action has all states in its from list, because an upsert could update anything in any state. (this could be evolved later on)

@enoonan
Copy link
Contributor Author

enoonan commented Nov 25, 2024

Just pushed a commit for a compile time check that create ... upsert? true actions allow transitions from all states.

I'm having a hard time wrapping my head around this:

...if the action is an upsert action, we now have to look up the valid next states for all transitions for this action and make sure that the given state is in that list.

Did you mean "we have to look up the valid next states for all transitions for this resource"? In my current understanding each action can have only one transition.

If so does that mean we'd need to check for a configuration like this?

    transitions do
      transition(:begin, from: :pending, to: [:executing, :pending])
      transition(:complete, from: :executing, to: [:complete, :pending])
      transition(:initialize, from: :*, to: :pending)
      ...
    end

@zachdaniel
Copy link
Contributor

There can be multiple transitions per action. For example, you could have:

transition(:begin, from: :pending, to: :executing)
transition(:begin, from: :paused, to: :resumed)

This is different than

transition(:begin, from: [:pending, :paused], to: [:executing, :resumed])

because the latter allows going from paused to executing, whereas the former does not. So you'd look up the possible destinations for all transitions for this action, and make sure that the target is in one of the lists.

@enoonan
Copy link
Contributor Author

enoonan commented Nov 25, 2024

Alrighty, just pushed that!

@zachdaniel
Copy link
Contributor

@enoonan looks like a small credo issue. You can add @moduledoc false and then I'll merge this.

@enoonan
Copy link
Contributor Author

enoonan commented Nov 26, 2024

Added!

@zachdaniel zachdaniel merged commit cbce39e into ash-project:main Nov 26, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants